Add support for Collection[X] | Y (for some collection types)#745
Add support for Collection[X] | Y (for some collection types)#745jhominal wants to merge 1 commit intopython-attrs:mainfrom
Conversation
|
Just letting you know this is on my radar, I'm feeling under the weather this week so it might have to wait a little |
|
Thank you for commenting about your status! Do not worry about it - there is really nothing on the line here except a possible improvement to users of |
Tinche
force-pushed
the
dispatch-single-collection
branch
from
e8add33 to
feb2f5b
Compare
Tinche
left a comment
Tinche
left a comment
There was a problem hiding this comment.
Looks very solid to me. Please proceed with the changelog entry and some documentation.
| ) -> Any: | ||
| # Design choice: only detect known concrete types as valid source types | ||
| # That avoids having to blacklist e.g. str or bytes | ||
| if isinstance(val, (deque, frozenset, list, set, tuple)): |
There was a problem hiding this comment.
I was going to ask if we can test against the exact type we're expecting, but that might be too restrictive. It's very common to structure lists into tuples/frozensets etc. So I think this is perfectly fine.
| # Design choice: only detect known concrete types as valid source types | ||
| # That avoids having to blacklist e.g. str or bytes | ||
| if isinstance(val, (deque, frozenset, list, set, tuple)): | ||
| return converter.structure(val, collection_type) |
There was a problem hiding this comment.
A trick you can do is finding these hooks in the hook factory itself (so in make_structure_union_single_collection, instead of in structure_union_single_collection) using converter.get_structure_hook, and then just using them here.
Unable to generate the performance reportThere was an internal error while processing the run's data. We're working on fixing the issue. Feel free to contact us on Discord or at support@codspeed.io if the issue persists. |
2 participants
fixes #743
As discussed in the issue, here is the PR.
Compared to my last comment on the issue:
deque,setandfrozenset. In case you would prefer not supporting these types, I can remove them (I am not sure whether the added support is "worth it" or not.)I must admit, I was not too inspired for tests, there are probably more tests that could be added. The PR is also missing documentation.